Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Oct 12, 2022

Summary

div is not an allowed child element of a hX tag, this leads to invalid behavior on firefox so we need to use the span tag as the NodeView content container.

Also using tabindex=-1 on the NodeView will trigger weird behavior when clicking inside the heading and pressing the enter key. It will split the node but also copy the heading anchor link of the NodeView.

@susnux susnux added bug Something isn't working 3. to review labels Oct 12, 2022
@juliusknorr
Copy link
Member

Wow, nice, thanks a lot @susnux. Tested and seems to work. Though the cypress failure puzzles me a bit as at least in the browser the id is properly set

@susnux susnux force-pushed the fix/heading-enter branch from a5d521b to 16cfa6d Compare October 12, 2022 12:15
@susnux
Copy link
Contributor Author

susnux commented Oct 12, 2022

/compile

@juliusknorr
Copy link
Member

/backport to stable25

susnux and others added 3 commits October 12, 2022 17:57
`div` is not an allowed child element of a `hX` tag,
this leads to invalid behavior on firefox so we need to use the `span` tag
as the NodeView content container.

Also using `tabindex=-1` on the NodeView will trigger weird behavior when
clicking inside the heading and pressing the enter key. It will split the node
but also copy the heading anchor link of the NodeView.

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@juliusknorr
Copy link
Member

I figured out that the -webkit-user-modify seems to cause quite some side-effects, so I pushed a fix to revert this change.

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Copy link
Member

Regarding cypress it seems that the way we clear the content editable is not very reliable in combination with the heading node view, so I moved the failing tests over to start with an empty file by default to avoid that. Something we should investigate separately in a follow up to maybe unify that logic or find a more stable way to clear the content.

@juliusknorr
Copy link
Member

/compile

@juliusknorr
Copy link
Member

File as follow up issues:

Signed-off-by: nextcloud-command <[email protected]>
@max-nextcloud max-nextcloud merged commit 74300dc into master Oct 13, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/heading-enter branch October 13, 2022 05:56
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@juliusknorr
Copy link
Member

juliusknorr commented Oct 13, 2022

Thanks a lot @susnux for the quick fix. Your contributions are highly appreciated. ❤️

On a separate note, we have a few public chat rooms on our Nextcloud instance, one also for text development, to stay in closer contact with community contributors and I thought it would also be cool to have you there. In case you are interested, just drop me a mail with your email address, then I can send you an invite. (Mail address is on my GitHub profile)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enter after headline removes the text

5 participants